-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Enforce indentation off-side rule #10691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Ref lampepfl/dotty 10671
Ref lampepfl/dotty 10671 Currently class A: val x = 1 val y = 2 is admitted. This becomes confusing since this looseness ends up not catching things like class Test: test("hello") assert(1 == 1) where `assert(1 == 1)` is actually not passed into `test(...)(...)`. Following the convention of indentation language like Python, we should reject superfluous indentation, which can be defined as having two or more indentation widths within a region.
Dogfooding / bootstrapping currently yields errors like Error: -- Error: /home/runner/work/dotty/dotty/library/src/scala/runtime/stdLibPatches/language.scala:36:0
Error: 36 | object noAutoTupling
Error: |^^^^^^
Error: |unexpected indent: found 0 spaces expected 2 spaces
Error: one error found but I'm not sure if it's an error in this code or in the existing scanner code since "found 0 spaces" seems wrong. |
This would have to go through a thorough review, and I have already stated the problems with this approach in the issue. My current feeling is, it will certainly not fly at the present time since it breaks too much code. The point to revisit would be if we make -Yindent-colons the default. But even then I would make the check semantics based, not syntax. I think there's real value in writing val x = 1
a
|| b
|| c Some people want to format things this way. |
Too much which code? My intent is that within a curly brace, we should continue to allow superfluous indentation. Here's a unit test I added for that: @Test
def parseBraces: Unit =
val code = s"""
|class A {
| val x = 1
| val y = 2
|}""".stripMargin
assert(parseTextEither(code).isRight) but not allow it when the user switch to indentation style: @Test
def superfluousIndents2: Unit =
val code = s"""
|class Test:
| test("hello")
| assert(1 == 1)
|""".stripMargin
assert(parseTextEither(code).isLeft)
I disagree on this point. I think, in many serious code base these variances are already formatted away by Scalafmt nowadays, and most people are happy to never think about it again. I see syntactically significant whitespace to be the next step in the continuum to make the shape of the code look the same regardless of who writes it, which is great for readability.
class Test:
test("hello")
assert(1 == 1) If indentation unambiguously meant introducing |
I agree that sometimes code that's indented too much can be misleading. But it's also often useful to allow it. Here is a situation I came across yestderday. I had ...
if someCondition then
doSomething and I wanted to comment out the ...
// if someCondition then
doSomething That was not meant to be permanent, just to try out things. Would that have worked with the proposed offside rule? |
Probably not, but then again it wouldn't have worked anyway if the thing just above the if cond1 then
stat1
//if cond2 then
stat2 In an indentation-based language, unfortunately the easiest way to avoid that problem is to do: if cond1 then
stat1
//if cond2 then
if true then
stat2 |
There was no activity on this one for a long while, so let's close it. |
I didn't notice that this was a PR on phone. #10671 is still open as bug. |
Ref #10671
Currently
is admitted. This becomes confusing since this looseness ends up not
catching things like
where
assert(1 == 1)
is actually not passed intotest(...)(...)
.Following the convention of indentation languages like Python, we should reject superfluous indentation, which can be defined as having two or more indentation widths within a region.
Test output